-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add secureFieldsWithDetails 😋 #2241
Conversation
Signed-off-by: zFernand0 <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #2241 +/- ##
=======================================
Coverage 91.30% 91.30%
=======================================
Files 632 632
Lines 18147 18147
Branches 3888 3849 -39
=======================================
Hits 16570 16570
Misses 1576 1576
Partials 1 1 ☔ View full report in Codecov by Sentry. |
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
Signed-off-by: zFernand0 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Fernando! I left a suggestion for the opts
parameter, but the implementation LGTM.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just have two little questions that don't need to be resolved.
config.mSecure = secureConfigs; | ||
expect(config.api.secure.secureFieldsForLayer(projectConfigPath)).toEqual({ [securePropPath]: "area51" }); | ||
expect(config.api.secure.secureFieldsForLayer(projectUserConfigPath)).toEqual(null); | ||
config.mSecure = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is being a little picky, but is this line necessary for the locally scoped config
variable?
getPropAttr("rejectUnauthorized", undefined, "boolean"), | ||
]); | ||
|
||
profInfo.getTeamConfig().mSecure = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, is this necessary for the locally scoped profInfo
?
* @param layerPath Path of the layer to get secure properties for | ||
* @returns the secure properties for the given layer, or null if not found | ||
*/ | ||
public secureFieldsForLayer(layerPath: string): IConfigSecureProperties { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is adding this method necessary? I'm probably being dumb but it seems like the secureFields
method has a layer parameter which can be used to filter the secure fields returned for only a specific layer.
What It Does
How to Test
Review Checklist
I certify that I have:
Additional Comments
Here is what the output will look like in case we would like to reuse the same
IProfArgAttrs
interface.